Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

hot region schedule supports store weight #2240

Closed
wants to merge 12 commits into from
Closed

hot region schedule supports store weight #2240

wants to merge 12 commits into from

Conversation

sourcelliu
Copy link
Contributor

@sourcelliu sourcelliu commented Mar 13, 2020 •
edited
Loading

What problem does this PR solve?

UCP #2168

What is changed and how it works?

Add a method for calculate the storeWeight

func calcStoreWeight(regionWeight, leaderWeight float64) float64 {
	storeWeight := (regionWeight + leaderWeight) / 2
	return math.Max(storeWeight, minWeight)
}

byteRate and KeyRate are recalculated using:

ByteRate: byteRate / storeWeight,
KeyRate:  keyRate / storeWeight,

Check List

Tests

  • Unit test

  • Manual test
    Command: sysbench oltp_read_only (table size=10000000, threads=300)
    before
    1before
    2before

after set store 1 hot-weight=1 store 2 hot-weight=1 store 7 hot-weight =2
1after
2after

Side effects

  • Increased code complexity

@sre-bot
Copy link
Contributor

sre-bot commented Mar 13, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 1294 points.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Mar 13, 2020
@sourcelliu
Copy link
Contributor Author

/run-all-tests

@nolouch nolouch added the component/schedule Scheduling logic. label Mar 14, 2020
@nolouch
Copy link
Contributor

nolouch commented Mar 16, 2020

PTAL @rleungx @disksing

nolouch
nolouch reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mantuliu, thanks for your contribution.
Could you do more tests or test it in a real cluster?

server/schedulers/hot_region.go Outdated
@@ -288,12 +297,17 @@ func summaryStoresLoad(
}
}

//calculate the storeWeight
storeWeight := calcStoreWeight(
cluster.GetStore(id).GetRegionWeight(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This weight may need to be separated from the cluster weight, because this is related to load, and the previous weight is more related to size. you can wait for #2239.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.I will do more test and wait for #2239.

server/schedulers/hot_region.go Outdated
return false
}
} else {
byteRkCmp := rankCmp(bs.cur.srcPeerStat.GetByteRate(), old.srcPeerStat.GetByteRate(), stepRank(0, 100))
keyRkCmp := rankCmp(bs.cur.srcPeerStat.GetKeyRate(), old.srcPeerStat.GetKeyRate(), stepRank(0, 10))
byteRkCmp := rankCmp(bs.cur.srcPeerStat.GetByteRate()/storeWeight, old.srcPeerStat.GetByteRate()/oldStoreWeight, stepRank(0, 100))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe do it in stepRank? compareSrcStore and compareDstStore also need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't implement this in the stepRank method, because stepRank might become something like the following:

func stepRank(rk0 float64, step float64) func(float64) int64 {
   return func(rate storeWeight float64) int64 {
      return int64((rate/storeWeight - rk0) / step)
   }
}

This makes all the methods related to stepRank need to be changed,such as stLdByteRate,stLdKeyRate,sliceLoadCmp,sliceLPCmp. I personally think that such changes will increase the complexity of these functions. But I still tried to optimize this logic and added a getRateWithStoreWeight function.

what do you think?^_^

nolouch
nolouch reviewed Mar 19, 2020
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, I will take the time to take a look and test.

@codecov-io
Copy link

codecov-io commented Mar 20, 2020 •
edited
Loading

Codecov Report

Merging #2240 into master will increase coverage by 0.14%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2240      +/-   ##
==========================================
+ Coverage   76.25%   76.39%   +0.14%     
==========================================
  Files         199      199              
  Lines       21591    21722     +131     
==========================================
+ Hits        16464    16595     +131     
+ Misses       3891     3888       -3     
- Partials     1236     1239       +3
Impacted Files Coverage Δ
server/api/store.go 65.01% <39.13%> (-2.30%) ⬇️
tools/pd-ctl/pdctl/command/component_command.go 63.54% <58.33%> (-0.51%) ⬇️
server/cluster/cluster.go 81.12% <66.66%> (+0.25%) ⬆️
tools/pd-ctl/pdctl/command/store_command.go 65.75% <66.66%> (+0.05%) ⬆️
server/core/storage.go 73.78% <77.77%> (+0.04%) ⬆️
server/schedulers/hot_region.go 82.55% <94.28%> (+0.58%) ⬆️
pkg/mock/mockcluster/mockcluster.go 91.25% <100.00%> (+0.34%) ⬆️
server/api/router.go 91.28% <100.00%> (+0.09%) ⬆️
server/config_manager/config_manager.go 81.97% <100.00%> (+0.37%) ⬆️
server/core/store.go 78.91% <100.00%> (+0.29%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c57c39d...e93f9b2. Read the comment docs.

@sourcelliu
Copy link
Contributor Author

Thanks for your contribution, I will take the time to take a look and test.

Thank you,I added a new parameter(hot-weight) to measure the ability of the store to handle hot regions. I will test it in an actual cluster.

@nolouch
Copy link
Contributor

nolouch commented Mar 23, 2020

Hi, @mantuliu thanks for your contribution.
We invite you to join our sig-scheduling, you can communicate with us more conveniently in slack channel.

@sourcelliu
Copy link
Contributor Author

@nolouch Thank you. I have joined.

rleungx
rleungx reviewed Mar 23, 2020
View reviewed changes
Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

server/schedulers/hot_test.go Outdated Show resolved Hide resolved
@nolouch
Copy link
Contributor

nolouch commented Apr 2, 2020

Hi @mantuliu .
Recently, we have adjusted some hotspot scheduling strategies, which may be a little different from your previous version. Would you resolve the conflict firstly？thanks :)

@sourcelliu
Copy link
Contributor Author

Hi @mantuliu .
Recently, we have adjusted some hotspot scheduling strategies, which may be a little different from your previous version. Would you resolve the conflict firstly？thanks :)

OK,I will resolve the conflict and make a test.

@sourcelliu sourcelliu closed this May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleungx rleungx rleungx left review comments

@nolouch nolouch nolouch left review comments

Assignees
No one assigned
Labels
component/schedule Scheduling logic. contribution This PR is from a community contributor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sourcelliu @sre-bot @nolouch @codecov-io @rleungx